Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[home] Implement backend queries & restyle StoryPreview component #34

Merged
merged 5 commits into from
Nov 14, 2023
Merged

[home] Implement backend queries & restyle StoryPreview component #34

merged 5 commits into from
Nov 14, 2023

Conversation

akshaynthakur
Copy link
Collaborator

@akshaynthakur akshaynthakur commented Nov 3, 2023 •
edited
Loading

What's new in this PR

  • Update StoryCard component to match post-mph designs
    • Update home screen and search screen to use the new styles
  • Update home screen heading to fetch user's username and display it
  • Add gear icon for settings button
  • Resize fonts in globalStyles to improve readability on home screen
  • Implement query to fetch story previews from featured_stories table
  • Implement query to fetch featured stories description
  • Implement new type StoryCard
    • Implement query to fetch story cards for recommended carousel
    • Implement query to fetch story cards for new stories carousel

Relevant Links

Notion Sprint Task

N/A

Online sources

N/A

Related PRs

Previous home screen frontend PR: #19

How to review

  • Need to test on iOS and different screen sizes -> try all ScrollViews, check spacing, borders, sizing, readability, drop shadows etc.
  • Will not see username displayed in the heading unless manually added in Supabase
    • Once signup/login with username is implemented this should work
  • May have an unhandled promise rejection for fetchUsername?

Next steps

  • Get design for loading screen and implement
  • Update schema to reflect fact that all featured stories will have 1 shared description
  • Recommended stories query: currently just chooses 5 random stories -> can consider refining into a better algorithm
  • Need to implement see all buttons for all 3 carousels -> new designs needed?
  • Implement onPress functionality for all stories
  • Either modify StoryPreview component or find new/simpler way to implement tags toggle view
  • Search screen: need to resolve globalStyles.container issue (squashes the search bar) so that drop shadows on StoryPreviews aren't cut off
  • May have an unhandled promise rejection for fetchUsername?

Tests Performed, Edge Cases

N/A

Screenshots

Screenshot_20231103-015908
Screenshot_20231103-015910
Screenshot_20231103-015917

CC: @akshaynthakur @sauhardjain @surajr10

@akshaynthakur akshaynthakur mentioned this pull request Nov 7, 2023
Merged
sauhardjain
sauhardjain reviewed Nov 8, 2023
View reviewed changes
Copy link
Contributor

@sauhardjain sauhardjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. A couple styling issues I noticed:

  • There is a white box on top of the bottom nav on iOS.
  • The buttons don't render properly on iOS.
  • The dropshadows are cut out on the right of the search screen on both iOS and Android.
  • More of a design call but the carousels aren't apparent to the user because the two cards fit perfectly on the screen after reloading. You could either adjust the sizing of these cards or add an arrow?

Upto you, if you'd want to handle these now or later but just wanted to point these out.

IMG_3984
IMG_3983

src/queries/profiles.tsx
Comment on lines +14 to +16
} else {
return data[0].username as string;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just highlighting that we should ensure username will actually be a string and not undefined here

src/app/(tabs)/home/index.tsx Outdated
Comment on lines 31 to 49
useEffect(() => {
(async () => {
const usernameResponse = await fetchUsername(user?.id);
setUsername(usernameResponse);
const featuredStoryResponse: StoryPreview[] =
await fetchFeaturedStoryPreviews();
setFeaturedStories(featuredStoryResponse);
const featuredStoryDescriptionResponse: string =
await fetchFeaturedStoriesDescription();
setFeaturedStoriesDescription(featuredStoryDescriptionResponse);
const recommendedStoriesResponse: StoryCard[] =
await fetchRecommendedStories();
setRecommendedStories(recommendedStoriesResponse);
const newStoriesResponse: StoryCard[] = await fetchNewStories();
setNewStories(newStoriesResponse);
})().finally(() => {
setLoading(false);
});
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the fetches do not depend on each other, we can just use a Promise.all to fetch these concurrently.

I would also recommend putting this in a try-catch block. If there are any expected errors here, and we'd want to still display the other responses then we should introduce an error state. This would also us to properly render error messages and still render the other values appropriately

@akshaynthakur akshaynthakur merged commit 63a41f6 into main Nov 14, 2023
2 checks passed
@akshaynthakur akshaynthakur deleted the akshay/update-home branch November 14, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sauhardjain sauhardjain sauhardjain left review comments

@surajr10 surajr10 Awaiting requested review from surajr10

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@akshaynthakur @sauhardjain